Add Disable Global Continuation Lists option #16421

LinHu2016 · 2022-12-06T15:13:37Z

The Continuation Buffers/Lists keep all of "live" Continuation Objects
and be maintained by GC in order to free up the related native structure
and java stacks in case the Continuation Object has not been terminated
normally.

currently the global Virtual Thread List will keep all of unterminated
Continuation Objects traceable, so Continuation Buffers/Lists are not
useful for current case, can disable it by XXgc:disableContinuationList
for saving extra scan.
cycle.

new XXgc options
disableContinuationList,
enaleContinuationList,
verifyContinuationList -- (default) verify if there is an exceptional
case.

Signed-off-by: Lin Hu linhu@ca.ibm.com

amicic · 2022-12-08T16:49:47Z

runtime/gc_glue_java/MarkingSchemeRootClearer.cpp

@@ -331,7 +331,7 @@ MM_MarkingSchemeRootClearer::scanContinuationObjects(MM_EnvironmentBase *env)
 							} else {
 								/* object was not previously marked */
 								gcEnv->_markJavaStats._continuationCleared += 1;
-								VM_VMHelpers::cleanupContinuationObject((J9VMThread *)env->getLanguageVMThread(), object);
+								VM_VMHelpers::cleanupContinuationObject((J9VMThread *)env->getLanguageVMThread(), object, (MM_GCExtensions::verify_continuation_list == _extensions->continuationListOption));


is this really necessary? wouldn't shouldScanContinuationObjects return false (since the lists are empty)?

ah sorry, I was hasty... this is about passing verify not about disabling

amicic · 2022-12-08T16:53:24Z

runtime/oti/VMHelpers.hpp

 	{
 #if JAVA_SPEC_VERSION >= 19
-		vmThread->javaVM->internalVMFunctions->freeContinuation(vmThread, objectPtr);
+		if (verifyOnly) {


I don't think it's 'only'. you also have to free it up in verify mode, otherwise there is a leak

for verify mode usage, we assume there is no case we can free up, so we assert when we find a case to free up.

ah, you are right, I was wrong.
this is GC path only (I was probably thinking we are in a common path, that last unmount uses, too)

amicic · 2022-12-08T16:55:14Z

runtime/oti/VMHelpers.hpp

-		vmThread->javaVM->internalVMFunctions->freeContinuation(vmThread, objectPtr);
+		if (verifyOnly) {
+			J9VMContinuation *continuation = J9VMJDKINTERNALVMCONTINUATION_VMREF(vmThread, objectPtr);
+			assert(NULL == continuation);


isn't there an API like Assert_VM_true ? or you could not #include it here?

Adding assertion to .hpp might be problematic

we can try to move the method to ContinuationHelpers.cpp

we already attempted this move (most of Continuation specific method should not really be here in a generic VM Helpers file) and there were problems, but still, give a thought/try again

there is a compiling setting problem to reference from subfolder oti to subfolder vm

we could pass the mode to freeContinuation, but I'm not very much fan of it (since it's shared with VM path of last unmount)

since this is GC specific helper, this could (in fact, should) be moved to a GC *.cpp file

can't find a good file to move it to, but so far best is base ObjectAccessBarrier
some others are ObjectModel, modronapi, VMInterfaceAPI, GCExtensions, FinalizerSupport...

probably the best is ContinuationObjectList, since this is already the list that we iterate just before calling the cleanup

as part of the move, change the signature, so that you pass Env, and the method itself will fetch VMThread

we should consider moving needScanStacksForContinuation and various related helpers, too, but not at as a part of this change

amicic · 2022-12-08T16:58:06Z

runtime/gc_base/modronapi.cpp

-	env->getGCEnvironment()->_continuationObjectBuffer->add(env, object);
+	if (MM_GCExtensions::disable_continuation_list != MM_GCExtensions::getExtensions(env)->continuationListOption) {
+		env->getGCEnvironment()->_continuationObjectBuffer->add(env, object);
+	}
 	MM_ObjectAllocationInterface *objectAllocation = env->_objectAllocationInterface;
 	if (NULL != objectAllocation) {
 		objectAllocation->getAllocationStats()->_continuationObjectCount += 1;


no point in doing this in disable mode

amicic · 2022-12-12T16:23:20Z

runtime/gc_base/GCExtensions.hpp

@@ -337,6 +344,7 @@ class MM_GCExtensions : public MM_GCExtensionsBase {
 		, minimumFreeSizeForSurvivor(DEFAULT_SURVIVOR_MINIMUM_FREESIZE)
 		, freeSizeThresholdForSurvivor(DEFAULT_SURVIVOR_THRESHOLD)
 		, recycleRemainders(true)
+		, continuationListOption(disable_continuation_list)


thinking if we have time to default to verify mode first for a few nights, and then disable it

amicic · 2022-12-12T17:17:31Z

runtime/gc_base/ContinuationObjectList.cpp

@@ -62,3 +62,18 @@ MM_ContinuationObjectList::addAll(MM_EnvironmentBase* env, j9object_t head, j9ob
 	MM_GCExtensions *extensions = MM_GCExtensions::getExtensions(env);
 	extensions->accessBarrier->setContinuationLink(tail, previousHead);
 }
+
+void
+MM_ContinuationObjectList::cleanupContinuationObject(MM_EnvironmentBase* env, j9object_t objectPtr)


now that this has a context (class scope) we can drop Continuation from the name.
we can also add Native in the name, but up to you. for example:
releaseNativesForObject

amicic · 2022-12-12T17:36:16Z

runtime/gc_vlhgc/GlobalMarkingScheme.cpp

 							env->_markVLHGCStats._continuationCleared += 1;
+							list->cleanupContinuationObject(env, object);


should be a static method and called with just a class scope.
like this it gives a false perception that list object is relevant
admittedly, moving this method to this class is not perfect, either

sorry for changing opinions.... but let's just move it to GCExtensions. we already touched it for the processing mode
in that case, the name of the method should still have 'Continuation'
for example releaseNativesForContinuationObject

we already have name like scanContinuationNativeSlots in various collector Java Delegate classes.
this cleanup method is common for all collectors, hence make sense to be in GCExtensions (which is just a global Java Delegate, although it does not have 'Delegate' in its name).
overall, both place (class owner) and method name is indeed consistent with scanContinuationNativeSlots

amicic · 2022-12-12T17:41:33Z

runtime/gc_base/modronapi.cpp

-	if (NULL != objectAllocation) {
-		objectAllocation->getAllocationStats()->_continuationObjectCount += 1;
+	if (MM_GCExtensions::disable_continuation_list != MM_GCExtensions::getExtensions(env)->continuationListOption) {
+		MM_EnvironmentBase *env = MM_EnvironmentBase::getEnvironment(vmThread->omrVMThread);


should be before 'if'

amicic · 2022-12-12T19:26:57Z

runtime/gc_base/GCExtensions.hpp

+
+		if (verify_continuation_list == continuationListOption) {
+		J9VMContinuation *continuation = J9VMJDKINTERNALVMCONTINUATION_VMREF(vmThread, objectPtr);
+		Assert_MM_true(NULL == continuation);


indentation

I think we should not add assertions to this .hpp file. We either move helper to .cpp file or create special slow path helper for assertion only in .cpp

or remove assertion obviously

I have no problem keeping it here, but don't know if compiler will have

you have particular concern since this is one of top level *hpp files?

yes, we had this problem in the past. Any place it is included it should have proper assertions setup. Currently we have no assertions in this file.

could you be more specific - we have asserts in some other *hpp files

Adding assertions to .hpp file without proper configuring them in this .hpp file creates hidden assumption (limitation) for environment this method is going to be called - the assertions should be configured there. An attempt to include .h file required for assertions in this file would cause multiple compilation errors across code

I think it's even does not matter where this method with assertion is called but the fact this code should be compiled everywhere it is included

amicic · 2022-12-12T19:31:01Z

runtime/oti/VMHelpers.hpp

@@ -22,6 +22,7 @@

 #if !defined(VMHELPERS_HPP_)
 #define VMHELPERS_HPP_
+#include <assert.h>


amicic · 2022-12-12T20:04:45Z

update the title (it's not disabled by default anymore)

amicic · 2022-12-12T20:33:51Z

runtime/gc_base/GCExtensions.cpp

+	J9VMThread *vmThread = (J9VMThread *)env->getLanguageVMThread();
+
+	if (verify_continuation_list == continuationListOption) {
+	J9VMContinuation *continuation = J9VMJDKINTERNALVMCONTINUATION_VMREF(vmThread, objectPtr);


indentation

amicic · 2022-12-12T20:47:04Z

runtime/gc_base/GCExtensions.cpp

+#if JAVA_SPEC_VERSION >= 19
+	J9VMThread *vmThread = (J9VMThread *)env->getLanguageVMThread();
+
+	if (verify_continuation_list == getExtensions(env)->continuationListOption) {


well, just make it a (non-static) member, then

amicic · 2022-12-12T20:54:24Z

runtime/gc_base/GCExtensions.cpp

+	J9VMContinuation *continuation = J9VMJDKINTERNALVMCONTINUATION_VMREF(vmThread, objectPtr);
+	Assert_MM_true(NULL == continuation);
+	} else {
+		vmThread->javaVM->internalVMFunctions->freeContinuation(vmThread, objectPtr);


this class should really have javaVM cached
interestingly, it has a javaVM() helper (that fetches it from omrVM each time) and it's used a number of times, but should just be a cached value
but let's be consistent at least and get it using javaVM() helper here, as well (of course, requires this to be a non-static member)
in future, if there is really a hot path that uses it, we can easily find them all and replace with a chashed value

The Continuation Buffers/Lists keep all of "live" Continuation Objects and be maintained by GC in order to free up the related native structure and java stacks in case the Continuation Object has not been terminated normally. currently the global Virtual Thread List will keep all of unterminated Continuation Objects traceable, so Continuation Buffers/Lists are not useful for current case, can disable it by XXgc:disableContinuationList for saving extra scan. cycle. - new XXgc options disableContinuationList, enaleContinuationList, verifyContinuationList -- (default) verify if there is an exceptional case. Signed-off-by: Lin Hu <linhu@ca.ibm.com>

amicic · 2022-12-13T03:17:15Z

jenkins compile win,aix jdk19

amicic · 2022-12-13T13:05:54Z

jenkins test sanity xlinux jdk19

LinHu2016 force-pushed the GC_Loom_update2 branch from 8fed167 to babaa45 Compare December 6, 2022 15:15

dmitripivkine added the comp:gc label Dec 6, 2022

amicic reviewed Dec 8, 2022

View reviewed changes

amicic approved these changes Dec 8, 2022

View reviewed changes

amicic self-requested a review December 8, 2022 17:47

LinHu2016 force-pushed the GC_Loom_update2 branch from babaa45 to 98340de Compare December 12, 2022 15:26